micropkg packaging improvements #2614
Conversation
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
2 times, most recently
from
829f2db
to
8b5087f
Compare
|
A small number of corner cases is failing, will have a look at those later this week. |
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
3 times, most recently
from
20bba12
to
f14db1a
Compare
|
The last set of test failures is due to a behavior change from I am hoping that we get an official answer soon, but while we wait, I think this is ready for review. |
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
from
f14db1a
to
2dd7074
Compare
There was a problem hiding this comment.
I'll need to look at this a second time, because there's quite a lot going on that I still need to process. I've left some initial thoughts/comments, but they're mostly nitpicks 🙈
General question is if this implementation is backwards compatible? (I'll also try it myself when re-reviewing)
I totally understand... 2622da6 could be reviewed separately, it's what makes things more complicated here. I could also split this in 2 PRs if it's easier for folks to review.
I didn't change any tests (only dropped one that was not needed anymore and modified error messages and pip invokations in others), so in principle this should be backwards compatible. |
|
Test failures hopefully addressed by https://github.com/pypa/packaging/pull/696/files (although I don't want to wait on that for this PR, we'll probably have to hack around it) |
|
I've tried packaging and pulling with this new version and the old (current) one and it all seems to work fine and to be backwards compatible 👌 It seems like the micropackage that's generated is still exactly the same. I guess that's supposed to be the case right? |
|
Yes, absolutely! That's why this PR would not fully close gh-2414 - the generated micropkg pretty much still uses |
There was a problem hiding this comment.
This looks like to be working as expected, so I'm happy to have it merged in. Maybe add to the release notes which changes were made here for traceability? And thanks for helping to improve this part of messy code base @astrojuanlu 🙇 ⭐
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
4 times, most recently
from
d4b6005
to
fedebbb
Compare
|
I have addressed all the outstanding comments and now I believe all tests should pass. If everything goes well and unless there are other opinions, I'll merge this. |
|
All tests passing, CI failures due to non-100 % coverage. Will need to add tests to cover the extra possible (but unlikely) error messages. |
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Fix gh-2542. Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
See gh-2414. Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Fix gh-2567. Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
See pypa/packaging#644 (comment) Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
from
fedebbb
to
f39a024
Compare
|
Added tests, finally! This is ready to go in. It has 2 approvals already but waiting until EOD to merge. |
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
astrojuanlu
force-pushed
the
feat/micropkg-packaging-improvements
branch
from
fde96bb
to
5387918
Compare
Ideally another engineer would approve this since the majority of the changes are in code. |
There was a problem hiding this comment.
Overall, the changes you've made to the micro-packaging workflow seem comprehensive and well thought out. Just some minor comments about readability.
Thank you @astrojuanlu
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Description
.egg-infodirectories forkedro micropkg#2567.kedro micropkg pullrelies on sdists #2542.pyproject.tomlinstead ofsetup.pyforkedro micropkg#2414 (still generates asetup.pyon the fly, but the code is ready to use apyproject.tomlinstead).pkg_resources, which is deprecated and will go away any time.Development notes
All the packaging stuff is quite complex - in fact, despite an initially good streak, this is now my second backtrack attempt, after opening gh-2569 and gh-2597 (hopefully those two can go next).
Keeping the micropackaging workflow intact while removing the strong assumptions around
setup.pywas extra hard, but I think I managed while staying within the "blessed" way of doing things. I could not avoid a runtime dependency on setuptools for now though, so we'll have to keep it around, maybe as an extra (gh-2350).It's been mentioned that this workflow is weird and that we should evaluate whether we want to keep it or not https://github.com/kedro-org/kedro/milestone/21 but this work blocks the rest of the progress in https://github.com/kedro-org/kedro/milestone/36, so I didn't want to block it on that decision (which could be taken after 0.19).
Checklist
RELEASE.mdfile